-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Safety comments: PR2 #864
Merged
Merged
Safety comments: PR2 #864
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
squell
force-pushed
the
safety-comments-2
branch
from
September 2, 2024 23:38
4002a79
to
9b2868e
Compare
squell
force-pushed
the
safety-comments-2
branch
2 times, most recently
from
September 3, 2024 00:02
70a8cdd
to
d0259ae
Compare
pvdrz
previously requested changes
Sep 26, 2024
squell
force-pushed
the
safety-comments-2
branch
2 times, most recently
from
October 7, 2024 20:41
4510210
to
641854e
Compare
bjorn3
reviewed
Oct 8, 2024
bjorn3
reviewed
Oct 8, 2024
bjorn3
reviewed
Oct 9, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looked through the entire PR now. I've got a couple of comments, but other than that LGTM.
squell
commented
Oct 9, 2024
squell
force-pushed
the
safety-comments-2
branch
from
October 9, 2024 13:22
e823c95
to
20994d3
Compare
bjorn3
reviewed
Oct 9, 2024
squell
force-pushed
the
safety-comments-2
branch
2 times, most recently
from
October 14, 2024 12:41
5eb18ad
to
833774d
Compare
bjorn3
approved these changes
Oct 14, 2024
squell
force-pushed
the
safety-comments-2
branch
2 times, most recently
from
October 22, 2024 08:16
ac3fe22
to
cdd29a7
Compare
- it was incorrect to label tcsetattr_nobg as a safe fn - clarify why the signal handler is safe
…struct the code flow
The si_pid field would be logged even when it is uninitialized. It also obvious that it isn't possible to trick sudo into thinking that the signal it received was from itself due to the uninitialized value it's own pid by chance. This is not possible as the check is prefixed by is_user_signaled and all signals where is_user_signaled are true have si_pid initialized. Using an option ensures that this check is never forgotten.
Co-authored-by: bjorn3 <[email protected]>
squell
force-pushed
the
safety-comments-2
branch
from
October 22, 2024 09:08
cdd29a7
to
6f25af5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is related to PR #860. Together with that PR it will close #861.
This PR also refactors some code in the signal handling, since there was code duplication going.
There are 3 missing
unsafe
statements after this:exec
(related topre_exec
)system
(related tofork
andpre_exec
)Anything relating to
fork
andpre_exec
probably needs special attention so I did not annotate those yet; for those please refer to issue #863There are also two unsafe statements in the
visudo
module, but that is handled by issue #859.